fix(ecr): autoDeleteImages fails when repository is renamed #26742

go-to-k · 2023-08-13T20:13:46Z

This PR fixes the bug that ECRAutoDeleteImages fails on repo rename.

The customResource depends on the role, and when the repository name changes, the role is updated to match the new repository instead of the old one, before customResource runs and the old repository is deleted.

It was difficult to delete the old repo before the role update ran, so I changed the resource of the role to a wildcard.

Closes #26711.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

go-to-k · 2023-08-13T20:20:09Z

Exemption Request because the unit tests covered changing role.

rix0rrr

Thanks for this!

I think a better solution for this would be add an AWS::IAM::Policy resource for every repository, and make sure the logical identifier of those resources is generated based off the repository name.

If we do that, standard CloudFormation sequencing rules will make everything work out correctly.

go-to-k · 2023-08-14T11:01:19Z

@rix0rrr

Thanks for comments!

I think a better solution for this would be add an AWS::IAM::Policy resource for every repository, and make sure the logical identifier of those resources is generated based off the repository name.

Does this mean using iam.Policy instead of addToRolePolicy?

new iam.Policy(this, `${this.repositoryName}Policy`, { // There may be differences in the rules between repositoryName and logical identifier
    roles: ... // specifying the new role instance generated from provider.roleArn
    statements: ... // for the repository
});

I still don't understand why this is good.

When the repository name is changed, the old repository is deleted, a new repository is created, and the images in the old repository are deleted by the custom resource.

At that time, wouldn’t the IAM policy then also be updated to match the new repository name, and eventually deletion of the old repository would no longer be allowed?

rix0rrr · 2023-08-14T11:54:31Z

Does this mean using iam.Policy instead of addToRolePolicy?

Yes.

Doing this, this.repositoryName may not be a token though so that may be hard to pull off.

I still don't understand why this is good.

Because of the order of CloudFormation resource operations during a stack update. There are two phases during a deployment:

CREATE/UPDATE
CLEANUP (in fact, DELETE)

So the policy that grants permissions to the old repository name will be deleted during the CLEANUP phase, when the actual ECR repository itself is also cleaned up.

I'm really not comfortable with the *.

If, because of the token-ness and unpredictability of this.repositoryName, this cannot work, a good compromise may be to put a tag on the ECR repository (cdk:allow-deleting-images=true; in fact there should already be a tag on there) and use tag-based access control with a Condition.

go-to-k · 2023-08-14T12:51:29Z

packages/aws-cdk-lib/aws-ecr/lib/repository.ts

      });
-    } else {
-      (provider as any)[REPO_ARN_SYMBOL].push(this._resource.attrArn);
    }


@rix0rrr

The repositoryName was Token. So following codes didn't work.

const role = iam.Role.fromRoleArn(this, `${this.repositoryName}Role`, provider.roleArn); // Use a iam policy to allow the custom resource to list & delete // images in the repository and the ability to get all repositories to find the arn needed on delete. new iam.Policy(this, `${this.repositoryName}Policy`, { roles: [role], statements: [ new iam.PolicyStatement({ effect: iam.Effect.ALLOW, actions: [ 'ecr:BatchDeleteImage', 'ecr:DescribeRepositories', 'ecr:ListImages', 'ecr:ListTagsForResource', ], resources: [this._resource.attrArn], }), ], });

If, because of the token-ness and unpredictability of this.repositoryName, this cannot work, a good compromise may be to put a tag on the ECR repository (cdk:allow-deleting-images=true; in fact there should already be a tag on there) and use tag-based access control with a Condition.

In that case, is this the way to go?

@rix0rrr

I used physicalName instead of repositoryName and it worked. Because this.repositoryName is a Token (lazy value), but physicalName is props.repositoryName.

However, props.repositoryName may be unspecified, in which case the props.repositoryName will be undefined and the IAM policy's logical IDs will duplicate. So this is not a good practice.

I agree, you can't use physicalName in this case. We should probably go the tag route then, a naked * is too dangerous for my tastes.

go-to-k · 2023-08-15T10:48:05Z

packages/aws-cdk-lib/aws-ecr/lib/repository.ts

+        Condition: {
+          StringEquals: {
+            ['ecr:ResourceTag/' + AUTO_DELETE_IMAGES_TAG]: 'true',
+          },
+        },


@rix0rrr

I implemented tag-based access control with a Condition for now.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2023-08-17T10:40:07Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-08-17T11:50:16Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-08-17T12:18:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 596ae0c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-08-17T12:18:57Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

go-to-k added 2 commits August 14, 2023 04:11

fix: change resources for the role

f0f912f

test: change integ snapshots

e344391

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Aug 13, 2023

aws-cdk-automation requested a review from a team August 13, 2023 20:13

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Aug 13, 2023

aws-cdk-automation previously requested changes Aug 13, 2023

View reviewed changes

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Aug 13, 2023

go-to-k marked this pull request as draft August 14, 2023 06:21

test: change integ tests

ae6cffa

go-to-k marked this pull request as ready for review August 14, 2023 07:03

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 14, 2023

rix0rrr requested changes Aug 14, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 14, 2023

go-to-k commented Aug 14, 2023

View reviewed changes

chore: use tag-based access control

21db9b8

go-to-k commented Aug 15, 2023

View reviewed changes

rix0rrr changed the title ~~fix(ecr): update ECRAutoDeleteImages role on repo rename~~ fix(ecr): autoDeleteImages fails when repository is renamed Aug 17, 2023

rix0rrr previously approved these changes Aug 17, 2023

View reviewed changes

rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 17, 2023

Merge branch 'main' into fix/ecr-auto-delete-images-role

22b20c9

rix0rrr approved these changes Aug 17, 2023

View reviewed changes

Merge branch 'main' into fix/ecr-auto-delete-images-role

596ae0c

mergify bot merged commit e264a2f into aws:main Aug 17, 2023
8 checks passed

go-to-k deleted the fix/ecr-auto-delete-images-role branch August 17, 2023 16:15

github-actions bot mentioned this pull request Sep 1, 2023

Monthly PRs metrics report - Aug 2023 #26965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecr): autoDeleteImages fails when repository is renamed #26742

fix(ecr): autoDeleteImages fails when repository is renamed #26742

go-to-k commented Aug 13, 2023

aws-cdk-automation left a comment

go-to-k commented Aug 13, 2023

rix0rrr left a comment

go-to-k commented Aug 14, 2023 •

edited

Loading

rix0rrr commented Aug 14, 2023

go-to-k Aug 14, 2023 •

edited

Loading

go-to-k Aug 15, 2023 •

edited

Loading

rix0rrr Aug 17, 2023

go-to-k Aug 15, 2023

mergify bot commented Aug 17, 2023

mergify bot commented Aug 17, 2023

aws-cdk-automation commented Aug 17, 2023

mergify bot commented Aug 17, 2023

fix(ecr): autoDeleteImages fails when repository is renamed #26742

fix(ecr): autoDeleteImages fails when repository is renamed #26742

Conversation

go-to-k commented Aug 13, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

go-to-k commented Aug 13, 2023

rix0rrr left a comment

Choose a reason for hiding this comment

go-to-k commented Aug 14, 2023 • edited Loading

rix0rrr commented Aug 14, 2023

go-to-k Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

go-to-k Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

rix0rrr Aug 17, 2023

Choose a reason for hiding this comment

go-to-k Aug 15, 2023

Choose a reason for hiding this comment

mergify bot commented Aug 17, 2023

mergify bot commented Aug 17, 2023

aws-cdk-automation commented Aug 17, 2023

AWS CodeBuild CI Report

mergify bot commented Aug 17, 2023

go-to-k commented Aug 14, 2023 •

edited

Loading

go-to-k Aug 14, 2023 •

edited

Loading

go-to-k Aug 15, 2023 •

edited

Loading